-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
delta-xds: avoid sending resource names for wildcard requests on stream reconnect #16153
delta-xds: avoid sending resource names for wildcard requests on stream reconnect #16153
Conversation
…am reconnect Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
grpc_mux_->onDiscoveryResponse(std::move(response), control_plane_stats_); | ||
} | ||
// Send another response with a different resource, but where EDS is paused. | ||
auto resume_cds = grpc_mux_->pause(type_url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add an update with a paused mux to this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done to emulate a disconnect before an ack is sent from the client, and ensure that the message on reconnect does include the new resource.
I'll add some comments to this test.
lgtm, other than a small question re: test. |
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…_resources Signed-off-by: Adi Suissa-Peleg <adip@google.com>
/assign @stevenzzzz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
api/xds_protocol.rst
Outdated
:ref:`initial_resource_versions <envoy_api_field_DeltaDiscoveryRequest.initial_resource_versions>`. | ||
Because no state is assumed to be preserved from the previous stream, the reconnecting | ||
client must provide the server with all resource names it is interested in. Note that for wildcard | ||
requests (e.g., CDS/LDS), the request must have no resources in both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
srds is wildcard as well.
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…_resources Signed-off-by: Adi Suissa-Peleg <adip@google.com>
/assign @htuch |
…am reconnect (envoyproxy#16153) Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>
…am reconnect (envoyproxy#16153) Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>
When a wildcard xDS type (LDS/CDS/SRDS) reconnects from a delta xDS stream, prior to envoy `1.19.0` it would populate the `ResourceNamesSubscribe` field with the full list of currently subscribed items, instead of simply omitting it to infer that it wanted everything (which is what wildcard mode means). This upstream issue was filed in envoyproxy/envoy#16063 and fixed in envoyproxy/envoy#16153 which went out in Envoy `1.19.0` and is fixed in later versions (later refactored in envoyproxy/envoy#16855). This PR conditionally forces LDS/CDS to be wildcard-only even when the connected Envoy requests a non-wildcard subscription, but only does so on versions prior to `1.19.0`, as we should not need to do this on later versions. This fixes the failure case as described here: #11833 (comment) Co-authored-by: Huan Wang <fredwanghuan@gmail.com>
When a wildcard xDS type (LDS/CDS/SRDS) reconnects from a delta xDS stream, prior to envoy `1.19.0` it would populate the `ResourceNamesSubscribe` field with the full list of currently subscribed items, instead of simply omitting it to infer that it wanted everything (which is what wildcard mode means). This upstream issue was filed in envoyproxy/envoy#16063 and fixed in envoyproxy/envoy#16153 which went out in Envoy `1.19.0` and is fixed in later versions (later refactored in envoyproxy/envoy#16855). This PR conditionally forces LDS/CDS to be wildcard-only even when the connected Envoy requests a non-wildcard subscription, but only does so on versions prior to `1.19.0`, as we should not need to do this on later versions. This fixes the failure case as described here: #11833 (comment) Co-authored-by: Huan Wang <fredwanghuan@gmail.com>
When a wildcard xDS type (LDS/CDS/SRDS) reconnects from a delta xDS stream, prior to envoy `1.19.0` it would populate the `ResourceNamesSubscribe` field with the full list of currently subscribed items, instead of simply omitting it to infer that it wanted everything (which is what wildcard mode means). This upstream issue was filed in envoyproxy/envoy#16063 and fixed in envoyproxy/envoy#16153 which went out in Envoy `1.19.0` and is fixed in later versions (later refactored in envoyproxy/envoy#16855). This PR conditionally forces LDS/CDS to be wildcard-only even when the connected Envoy requests a non-wildcard subscription, but only does so on versions prior to `1.19.0`, as we should not need to do this on later versions. This fixes the failure case as described here: #11833 (comment) Co-authored-by: Huan Wang <fredwanghuan@gmail.com>
When a wildcard xDS type (LDS/CDS/SRDS) reconnects from a delta xDS stream, prior to envoy `1.19.0` it would populate the `ResourceNamesSubscribe` field with the full list of currently subscribed items, instead of simply omitting it to infer that it wanted everything (which is what wildcard mode means). This upstream issue was filed in envoyproxy/envoy#16063 and fixed in envoyproxy/envoy#16153 which went out in Envoy `1.19.0` and is fixed in later versions (later refactored in envoyproxy/envoy#16855). This PR conditionally forces LDS/CDS to be wildcard-only even when the connected Envoy requests a non-wildcard subscription, but only does so on versions prior to `1.19.0`, as we should not need to do this on later versions. This fixes the failure case as described here: #11833 (comment) Co-authored-by: Huan Wang <fredwanghuan@gmail.com> Co-authored-by: Huan Wang <fredwanghuan@gmail.com>
Commit Message: delta-xds: avoid sending resource names for wildcard requests on stream reconnect
Additional Description:
Avoiding sending resource names for wildcard requests after the stream is dropped.
The resources are sent in
initial_resource_versions
, but the new request must have no resources inresource_names_subscribe
so the server will understand this is a wildcard request.Risk Level: Medium? breaks current behavior, which is incorrect.
Testing: Added unit and integration tests.
Docs Changes: Updated xds docs.
Release Notes: N/A.
Platform Specific Features: N/A.
Fixes #16063
Signed-off-by: Adi Suissa-Peleg adip@google.com